properly handle revision based_on field

Author: escattone

kitsune/wiki/migrations/0019_ensure_consistent_based_on.py

@@ -0,0 +1,35 @@
+# Generated by Django 4.2.20 on 2025-03-27 15:45
+
+from django.db import migrations
+
+
+def ensure_empty_based_on_for_non_translations(apps, schema_editor):
+    """
+    Although historically the "based_on" of revisions of documents without a
+    parent (mostly English documents but there also exist a few non-English
+    documents without a parent) has often been set to an earlier revision
+    within the same document, it has never been used for these revisions.
+    From this point forward, we want to ensure that the "based_on" of a
+    revision can be non-NULL only if the revision is a translation of an
+    English revision, or in other words, only for revisions of documents
+    with a parent. So we'll set the "based_on" to NULL for any revision
+    that is not a translation of an English revision, or in other words,
+    for any revision of a document without a parent.
+    """
+    Revision = apps.get_model("wiki", "Revision")
+    Revision.objects.filter(document__parent__isnull=True, based_on__isnull=False).update(
+        based_on=None
+    )
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("wiki", "0018_alter_document_restrict_to_groups"),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            ensure_empty_based_on_for_non_translations, migrations.RunPython.noop
+        ),
+    ]

kitsune/wiki/models.py

@@ -861,20 +861,21 @@ class Meta(object):
         ]
 
     def _based_on_is_clean(self):
-        """Return a tuple: (the correct value of based_on, whether the old
-        value was correct).
-
-        based_on must be a revision of the English version of the document. If
-        based_on is not already set when this is called, the return value
-        defaults to something reasonable.
+        """
+        Returns a tuple:
+            (the correct value of "based_on", whether the old value was correct)
 
+        The value of "based_on" should only be non-NULL for revisions of documents
+        with a parent, or in other words, revisions that are translations of an
+        English revision, and in those cases its value should always be a revision
+        of its document's parent (an English document).
         """
-        original = self.document.original
-        if self.based_on and self.based_on.document != original:
-            # based_on is set and points to the wrong doc. The following is
-            # then the most likely helpful value:
-            return original.localizable_or_latest_revision(), False
-        # Even None is permissible, for example in the case of a brand new doc.
+        if self.document.parent:
+            if not self.based_on or (self.based_on.document != self.document.parent):
+                return self.document.parent.localizable_or_latest_revision(), False
+        elif self.based_on:
+            return None, False
+
         return self.based_on, True
 
     def clean(self):
@@ -965,7 +966,9 @@ def latest_revision(excluded_rev, constraint):
             except IndexError:
                 return None
 
-        Revision.objects.filter(based_on=self).update(based_on=None)
+        # This is a safeguard, since these revisions should already have a "based_on" of None.
+        Revision.objects.filter(document__parent__isnull=True, based_on=self).update(based_on=None)
+
         document = self.document
 
         # If the current_revision is being deleted, try to update it to the

kitsune/wiki/tests/test_handlers.py

@@ -205,3 +205,68 @@ def test_mixed_revisions_user_deletion(self):
 
         rev2 = Revision.objects.filter(id=rev2_id).first()
         self.assertIsNone(rev2, "Deleted user's revision should be deleted")
+
+    def test_revision_based_on_deletion(self):
+        """
+        Test the handling of a revision's "based_on" field when it references a revision
+        created by a user that is deleted.
+        """
+        doc = DocumentFactory()
+        rev1 = RevisionFactory(document=doc, creator=self.user)
+        rev2 = RevisionFactory(document=doc, based_on=rev1)
+        rev3 = ApprovedRevisionFactory(document=doc, based_on=rev2)
+
+        doc_id = doc.id
+        rev1_id = rev1.id
+        rev2_id = rev2.id
+        rev3_id = rev3.id
+
+        self.listener.on_user_deletion(self.user)
+
+        self.assertTrue(
+            Document.objects.filter(id=doc_id).exists(), "Document should not be deleted"
+        )
+        self.assertFalse(
+            Revision.objects.filter(id=rev1_id).exists(),
+            "Deleted user's unapproved revision should be deleted",
+        )
+        rev2 = Revision.objects.filter(id=rev2_id).first()
+        self.assertIsNotNone(rev2, "Other user's revision should not be deleted")
+        self.assertIsNone(rev2.based_on, "Other user's revision's based_on should now be None")
+        rev3 = Revision.objects.filter(id=rev3_id).first()
+        self.assertIsNotNone(rev3, "The approved revision should not be deleted")
+        self.assertEqual(rev3.based_on, rev2)
+
+        it_doc = DocumentFactory(locale="it", parent=doc)
+        it_rev = RevisionFactory(document=it_doc, based_on=rev2)
+
+        it_doc_id = it_doc.id
+        it_rev_id = it_rev.id
+
+        self.listener.on_user_deletion(rev2.creator)
+
+        # Ensure that the default document wasn't deleted.
+        self.assertTrue(
+            Document.objects.filter(id=doc_id).exists(), "Document should not be deleted"
+        )
+
+        # The deleted user's un-approved revision should not have been deleted,
+        # but instead re-assigned to the sumo bot, because it's the based-on
+        # value of a localized revision.
+        rev2 = Revision.objects.filter(id=rev2_id).first()
+        self.assertIsNotNone(rev2, "Deleted user's revision should not be deleted")
+        self.assertEqual(rev2.creator.username, settings.SUMO_BOT_USERNAME)
+
+        # Ensure that the third revision, whose based-on value is the revision
+        # whose creator was deleted, was not affected at all.
+        rev3 = Revision.objects.filter(id=rev3_id).first()
+        self.assertIsNotNone(rev3, "The approved revision should not be deleted")
+        self.assertEqual(rev3.based_on, rev2)
+
+        self.assertTrue(
+            Document.objects.filter(id=it_doc_id).exists(),
+            "The Italian document should not be deleted",
+        )
+        it_rev = Revision.objects.filter(id=it_rev_id).first()
+        self.assertIsNotNone(it_rev, "The Italian revision should not be deleted")
+        self.assertEqual(it_rev.based_on, rev2)

kitsune/wiki/views.py

@@ -551,7 +551,7 @@ def edit_document(request, document_slug, revision_id=None):
         return init_check
     user, doc, rev = init_check
 
-    rev_form = RevisionForm(instance=rev, initial={"based_on": rev.id, "comment": ""})
+    rev_form = RevisionForm(instance=rev, initial={"based_on": None, "comment": ""})
 
     # POST
     if request.method == "POST":